Skip to content

Port #[should_panic] to the new attribute parsing infrastructure #143808

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Port #[should_panic] to the new attribute parsing infrastructure #143808

wants to merge 2 commits into from

Conversation

JonathanBrouwer
Copy link
Contributor

Ports #[should_panic] to the new attribute parsing infrastructure for #131229 (comment)

r? @jdonszelmann

@rustbot
Copy link
Collaborator

rustbot commented Jul 11, 2025

jdonszelmann is currently at their maximum review capacity.
They may take a while to respond.

@rustbot rustbot added A-attributes Area: Attributes (`#[…]`, `#![…]`) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 11, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jul 11, 2025

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

Some changes occurred in compiler/rustc_attr_data_structures

cc @jdonszelmann

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann

Some changes occurred in compiler/rustc_passes/src/check_attr.rs

cc @jdonszelmann

JonathanBrouwer
JonathanBrouwer commented Jul 11, 2025
View reviewed changes
@rustbot rustbot added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 11, 2025
JonathanBrouwer
JonathanBrouwer commented Jul 11, 2025
View reviewed changes
tests/ui/test-attrs/test-should-panic-attr.rs
@@ -1,4 +1,3 @@
//@ check-pass
Copy link
Contributor Author

@JonathanBrouwer JonathanBrouwer Jul 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a breaking change! Malformed should_panic attributes have had the following warning since 2016:

warning: argument must be of the form: `expected = "error message"`
 --> src/main.rs:5:1
  |
5 | #[should_panic(expected = "foo", bar)]
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: errors in this attribute were erroneously allowed and will become a hard error in a future release

This PR makes this an error.

As discussed in #142838 (comment), we can make breaking changes as long as we do a crater run. So this PR needs a crater run.

JonathanBrouwer
JonathanBrouwer commented Jul 11, 2025
View reviewed changes
Copy link
Contributor Author

@JonathanBrouwer JonathanBrouwer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This issue is currently NOT solved by this PR.
We can do this in a future PR or now, but we should decide whether this should be a warning or error:
#143799

@bors
Copy link
Collaborator

bors commented Jul 12, 2025

☔ The latest upstream changes (presumably #143810) made this pull request unmergeable. Please resolve the merge conflicts.

This was referenced Jul 12, 2025
Open
Open
@JonathanBrouwer
Copy link
Contributor Author

^ Rebased on #143796
@rustbot ready

I'd propose to do #143799 in a separate PR, as I think it's more likely to fail crater than the change in this PR

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Jul 13, 2025
@bors
Copy link
Collaborator

bors commented Jul 14, 2025

☔ The latest upstream changes (presumably #143779) made this pull request unmergeable. Please resolve the merge conflicts.

@JonathanBrouwer JonathanBrouwer force-pushed the should_panic_parser branch 2 times, most recently from bee8d3d to 4b68dc9 Compare July 15, 2025 11:59
@bors
Copy link
Collaborator

bors commented Jul 17, 2025

☔ The latest upstream changes (presumably #144044) made this pull request unmergeable. Please resolve the merge conflicts.

@jdonszelmann
Copy link
Contributor

@bors try

@bors
Copy link
Collaborator

bors commented Jul 17, 2025

🔒 Merge conflict

This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again.

How do I rebase?

Assuming self is your fork and upstream is this repository, you can resolve the conflict following these steps:

  1. git checkout should_panic_parser (switch to your branch)
  2. git fetch upstream master (retrieve the latest master)
  3. git rebase upstream/master -p (rebase on top of it)
  4. Follow the on-screen instruction to resolve conflicts (check git status if you got lost).
  5. git push self should_panic_parser --force-with-lease (update this PR)

You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial.

Please avoid the "Resolve conflicts" button on GitHub. It uses git merge instead of git rebase which makes the PR commit history more difficult to read.

Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Cargo.lock conflict is handled during merge and rebase. This is normal, and you should still perform step 5 to update this PR.

Error message 
Auto-merging compiler/rustc_passes/src/check_attr.rs
Auto-merging compiler/rustc_parse/src/validate_attr.rs
Auto-merging compiler/rustc_hir/src/hir.rs
Auto-merging compiler/rustc_builtin_macros/src/test.rs
CONFLICT (content): Merge conflict in compiler/rustc_builtin_macros/src/test.rs
Auto-merging compiler/rustc_attr_parsing/src/context.rs
Auto-merging compiler/rustc_attr_data_structures/src/encode_cross_crate.rs
Auto-merging compiler/rustc_attr_data_structures/src/attributes.rs
Automatic merge failed; fix conflicts and then commit the result.

@jdonszelmann
Copy link
Contributor

@bors try

bors added a commit that referenced this pull request Jul 17, 2025
@bors
Auto merge of #143808 - JonathanBrouwer:should_panic_parser, r=<try>
5de9e0e 
Port `#[should_panic]` to the new attribute parsing infrastructure

Ports `#[should_panic]` to the new attribute parsing infrastructure for #131229 (comment)

r? `@jdonszelmann`
@bors
Copy link
Collaborator

bors commented Jul 17, 2025

⌛ Trying commit 877a65d with merge 5de9e0e...

@bors
Copy link
Collaborator

bors commented Jul 17, 2025

☀️ Try build successful - checks-actions
Build commit: 5de9e0e (5de9e0e6cf775e94e0de2003f35e183f3f77bfe8)

@jdonszelmann
Copy link
Contributor

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-143808 created and queued.
🤖 Automatically detected try build 5de9e0e
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@jdonszelmann jdonszelmann

Labels
A-attributes Area: Attributes (`#[…]`, `#![…]`) S-waiting-on-crater Status: Waiting on a crater run to be completed. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@JonathanBrouwer @rustbot @bors @jdonszelmann @craterbot